Gate exception field serialization behind langversion 11#19746
Conversation
|
…FieldSerializationSupport (F# 11) The GetObjectData override and field-restoring deserialization constructor for exception types are now gated behind langversion 11. With langversion <=10 (the current default), exceptions emit only the base-call ctor (status quo ante PR #19342). - Add LanguageFeature.ExceptionFieldSerializationSupport, mapped to F# 11.0 - Gate shouldRestoreFields and GetObjectData emission on the feature - Update tests to use withLangVersion "11" - Update all IL baselines (SerializableAttribute, Nullness) to reflect the default-langversion output (no field serde IL) Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
16c595f to
733c88b
Compare
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Commit pushed:
|
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Commit pushed:
|
This comment has been minimized.
This comment has been minimized.
The net472 baseline was incorrectly updated to use [runtime] prefixed NullableContextAttribute and NullableAttribute references. On net472, these attributes are embedded in the assembly (not from runtime BCL), so they must not have the [runtime] prefix. Also restore the embedded attribute class definitions at the end of the file. The serialization changes (removing field-restoring ctor and GetObjectData) are correctly kept, as they match the default langversion behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Commit pushed:
|
This comment has been minimized.
This comment has been minimized.
T-Gro
left a comment
There was a problem hiding this comment.
Review: Gate exception field serialization behind langversion 11
This is a well-structured PR that properly gates the behavioral change from #19342 behind LanguageFeature.ExceptionFieldSerializationSupport (F# 11). The approach is sound — emitting GetObjectData and the field-restoring deserialization ctor changes the binary contract of exception types, so gating it is the right call for backward compatibility.
What's good
- Clean gating logic: The
emitFieldSerializationboolean at line 12320-12323 is easy to follow and correctly combines langversion check, FSharp.Core exclusion, and empty-fields short-circuit. - Test coverage: The runtime roundtrip tests and the
MessageFieldCollisionregression test explicitly opt into langversion 11. The FSharp.Core negative test (Issue_878_FSharpCoreExceptions_NoGetObjectDataOverride) correctly validates that FSharp.Core exceptions are still excluded. - Baseline IL updates: All baselines that compile at default langversion correctly no longer contain GetObjectData / field-restore IL — this proves the gate works for the default path.
- Release notes: Properly updated to mention the langversion gate.
- Code structure improvement: Moving
ilInstrsToSaveFields,ilInstrsForGetObjectData, andilGetObjectDataDefinside theif emitFieldSerialization thenbranch avoids constructing those values when they'll never be used.
Minor observations (non-blocking)
-
ilInstrsToRestoreFieldsstill computed unconditionally (lines 12286-12318): Unlike the save/GetObjectData side which was moved inside theif, the restore instructions are computed even whenemitFieldSerialization = false. The list is just discarded via theif ... then ilInstrsToRestoreFields else []at line 12332. Consider moving it inside the branch for symmetry and to avoid the allocation in the common (langversion < 11) path. Not a correctness issue. -
No explicit negative test: The IL baselines implicitly prove that default langversion doesn't emit field serialization, but an explicit
[<Fact>]that compiles an exception at langversion 10 and asserts absence of GetObjectData would be stronger documentation of intent. The existing coverage via Nullness/SerializableAttribute baselines is adequate though.
Overall: clean, minimal, correct.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Commit pushed:
|
|
🤖 LabelOps — CI fix. Added release notes entry to The
|
Gates the GetObjectData + field-restoring ctor from #19342 behind
LanguageFeature.ExceptionFieldSerializationSupport(F# 11).With langversion ≤10 (current default), exception codegen is unchanged from pre-#19342 behavior.